<html>
<head><meta charset="utf-8"><title>Review for bit-rotty PR? · t-libs · Zulip Chat Archive</title></head>
<h2>Stream: <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/index.html">t-libs</a></h2>
<h3>Topic: <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html">Review for bit-rotty PR?</a></h3>

<hr>

<base href="https://rust-lang.zulipchat.com">

<head><link href="https://rust-lang.github.io/zulip_archive/style.css" rel="stylesheet"></head>

<a name="216828563"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216828563" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Joshua Nelson <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216828563">(Nov 16 2020 at 03:01)</a>:</h4>
<p><a href="https://github.com/rust-lang/rust/pull/78934">https://github.com/rust-lang/rust/pull/78934</a> is splitting up <code>vec/mod.rs</code> (heh, I'm the one who <a href="https://github.com/rust-lang/rust/pull/68692">added <code>tidy-ignore-filelength</code></a> way back in March). Could someone review it before it runs into too many merge conflicts?</p>



<a name="216831165"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216831165" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Josh Triplett <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216831165">(Nov 16 2020 at 04:13)</a>:</h4>
<p>It's currently assigned to me, but I definitely don't have the bandwidth for a PR that size at the moment. Would appreciate someone else taking it.</p>



<a name="216853872"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216853872" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Ashley Mannix <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216853872">(Nov 16 2020 at 10:28)</a>:</h4>
<p>Hmm, I feel like PRs that are splitting up large modules into smaller child ones that use mostly <code>pub(super)</code> or <code>pub(crate)</code> internals could be missing an opportunity to define better privacy boundaries <span aria-label="thinking" class="emoji emoji-1f914" role="img" title="thinking">:thinking:</span></p>



<a name="216864005"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216864005" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Mara <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216864005">(Nov 16 2020 at 12:26)</a>:</h4>
<p>reassigned it to myself. i can look at it today or tomorrow</p>



<a name="216866497"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216866497" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Mara <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216866497">(Nov 16 2020 at 12:53)</a>:</h4>
<p>Reviewed. Added Ashley's comment too.</p>



<a name="216873663"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216873663" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Joshua Nelson <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216873663">(Nov 16 2020 at 13:59)</a>:</h4>
<p>Thank you!</p>



<a name="216875675"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216875675" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Tim Diekmann <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216875675">(Nov 16 2020 at 14:13)</a>:</h4>
<p>Is it possible to wait for <a href="https://github.com/rust-lang/rust/issues/78461">#78461</a>? It was a lot of work getting it to pass all the tests and I don't want another crater run. <span aria-label="smile" class="emoji emoji-1f642" role="img" title="smile">:smile:</span></p>



<a name="216877605"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216877605" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Mara <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216877605">(Nov 16 2020 at 14:28)</a>:</h4>
<p><span class="user-mention" data-user-id="216785">@Tim Diekmann</span> moving interal things around to different files wouldn't need another crater run.</p>



<a name="216877702"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216877702" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Mara <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216877702">(Nov 16 2020 at 14:28)</a>:</h4>
<p>anyway, it might be good to put a comment on that cleanup PR to say that there's already a big Vec change open right now</p>



<a name="216877735"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216877735" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Tim Diekmann <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216877735">(Nov 16 2020 at 14:28)</a>:</h4>
<p>No, but it's easy to mess something up or to forget something, when rebasing such a big PR.</p>



<a name="216877782"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216877782" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Tim Diekmann <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216877782">(Nov 16 2020 at 14:29)</a>:</h4>
<blockquote>
<p>anyway, it might be good to put a comment on that cleanup PR to say that there's already a big Vec change open right now</p>
</blockquote>
<p>Will add a comment then. Thank you!</p>



<a name="216877827"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216877827" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> simulacrum <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216877827">(Nov 16 2020 at 14:29)</a>:</h4>
<p>FWIW I am also personally pretty against splitting up files just because of tidy limitations</p>



<a name="216877847"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216877847" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> simulacrum <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216877847">(Nov 16 2020 at 14:29)</a>:</h4>
<p>i.e., I think the vec file is not really complicated because of its length</p>



<a name="216877908"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216877908" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> simulacrum <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216877908">(Nov 16 2020 at 14:30)</a>:</h4>
<p>each piece/impl is fairly small and easy to understand</p>



<a name="216877953"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216877953" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> simulacrum <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216877953">(Nov 16 2020 at 14:30)</a>:</h4>
<p>the length limit is mostly a "hm, maybe think about the structure" but in this case that's not really a concern</p>



<a name="216878122"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216878122" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Mara <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216878122">(Nov 16 2020 at 14:31)</a>:</h4>
<p>It looks like a nice cleanup to me. Especially putting things like Drain and DrainFilter with their trait impls together in their own file.</p>



<a name="216878432"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216878432" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> Tim Diekmann <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216878432">(Nov 16 2020 at 14:33)</a>:</h4>
<p>I worked a lot with this file recently. Splitting it up would have helped a lot.</p>



<a name="216878967"></a>
<h4><a href="https://rust-lang.zulipchat.com#narrow/stream/219381-t-libs/topic/Review%20for%20bit-rotty%20PR%3F/near/216878967" class="zl"><img src="https://rust-lang.github.io/zulip_archive/assets/img/zulip.svg" alt="view this post on Zulip" style="width:20px;height:20px;"></a> simulacrum <a href="https://rust-lang.github.io/zulip_archive/stream/219381-t-libs/topic/Review.20for.20bit-rotty.20PR.3F.html#216878967">(Nov 16 2020 at 14:37)</a>:</h4>
<p><span aria-label="thumbs up" class="emoji emoji-1f44d" role="img" title="thumbs up">:thumbs_up:</span> -- seems fine in this case, just wanted to say that "no" is also valid response :)</p>



<hr><p>Last updated: Aug 07 2021 at 22:04 UTC</p>
</html>